-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ci: Replace actions-rs actions in GitHub workflows #678
Conversation
|
d559f74
to
d322298
Compare
override: true | ||
|
||
- uses: Swatinem/rust-cache@v2 | ||
- uses: ./.github/actions/setup-rust |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
main
was already merged to feat/multichain
, so this should be safe to do, i.e. the action should already be there.
.github/workflows/edr-ci.yml
Outdated
components: clippy | ||
|
||
- uses: Swatinem/rust-cache@v2 | ||
run: rustup toolchain install nightly --profile minimal --component rustfmt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use inputs to the setup-rust
action?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found it to be a lot simpler - we just execute setup-rust
as our baseline "setup Rust toolchain as expected" and this is the only exception to this. Since it's a one-liner and used only here, I found it simpler to just call this directly and keep the main action lean.
EDIT: To expand a bit on the rationale, I did try that initially, but we'd have to be more explicit, i.e. support and pass override: false
for clarity, support and handle components
, support it being optional and pass some if missing (I do install always install clippy for simplicity as the cost is near-zero; unfortunately the overhead of cross-platform setup via bash is bigger when using this action vs actions-rs).
The nightly rustfmt is also hopefully only a temporary solution until upstream stabilises the two options that we use, so I did not want to come up with a principled approach to a one-off that's hopefully temporary anyway 🤞
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can have default values for all inputs of an action, which would keep the default action lean too, while allowing configuration.
I'll leave it up to you to decide whether to take that approach or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to find a middle ground in e7796f7, hope that's okay with you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! That's exactly what I had in mind :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for only getting to it now. I really like how you used cargo
directly ❤️ I only have one comment about potentially pinning action versions.
- name: Install Rust (stable) | ||
uses: actions-rs/toolchain@v1 | ||
- name: Install Rust | ||
uses: actions-rust-lang/setup-rust-toolchain@v1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest pinning the version of this action to a commit ref unless it comes from an organization we own or trust. Otherwise, we could open up an attack vector.
uses: actions-rust-lang/setup-rust-toolchain@4d1965c9142484e48d40c19de54b5cba84953a06 # v1.1.0
Closes #642
This gets rid of the redundant
actions-rs/cargo
and moves to the maintained action https://github.com/actions-rust-lang/setup-rust-toolchain instead ofactions-rs/toolchain
.